-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor of Allocator classes #9074
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9074 +/- ##
============================================
- Coverage 15.54% 4.18% -11.36%
============================================
Files 5494 372 -5122
Lines 481055 30448 -450607
Branches 59882 5398 -54484
============================================
- Hits 74769 1274 -73495
+ Misses 398023 29030 -368993
+ Partials 8263 144 -8119
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
1ff8f4a
to
42d16d9
Compare
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
537c10f
to
952c273
Compare
import com.cloud.utils.component.AdapterBase; | ||
import com.cloud.vm.VirtualMachine; | ||
import com.cloud.vm.VirtualMachineProfile; | ||
|
||
public class TestingAllocator extends AdapterBase implements HostAllocator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this not a BaseAllocator-child, @BryanMLima ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DaanHoogland, because TestingAllocator
does not have any methods in common with BaseAllocator
. Is there a reason that it should extend it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but it seems an outlier and the name also suggests it is intended to be a mock version for testing what would make less sense if it is not based on the same. I'd have to study it's use to give a definite answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I did some basic testing, changed the allocator to FirstFit and created some VMs, the VM allocation worked fine with and without tags. However, my tests were limited, further testing would be good.
dd1eb14
to
952c273
Compare
Description
This PR refactors some
*Allocator
classes, improving modularity and code legibility. This PR also made some changes to logs across this class.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?
I tested the allocation process in my personal lab, using both the
RandomAllocator
andFirstFitAllocator
allocators. I tried some variation of tags and offerings, and everything looks good. Furthermore, I also added a lot of unit tests for the methods that I refactored.How did you try to break this feature and the system with this change?